Skip to content

Fix access control for non-public templates (enforce ownership for non-admin users)#12683

Open
sudo87 wants to merge 2 commits intoapache:mainfrom
shapeblue:templatePermissionCheck
Open

Fix access control for non-public templates (enforce ownership for non-admin users)#12683
sudo87 wants to merge 2 commits intoapache:mainfrom
shapeblue:templatePermissionCheck

Conversation

@sudo87
Copy link
Copy Markdown
Contributor

@sudo87 sudo87 commented Feb 23, 2026 •
edited
Loading

Description

Permission access check looks incorrect for non-public template and non admin user.

Provided non-public templates are not shareable, check must ensure that template is owned by user. For public, ownership flag is passed as false, which makes sense as template can be created by other user.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 23, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 18.00%. Comparing base (30e6c22) to head (fe7fc5a).
⚠️ Report is 65 commits behind head on main.

Files with missing lines Patch % Lines
...ain/java/com/cloud/api/query/QueryManagerImpl.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main   #12683      +/-   ##
============================================
+ Coverage     17.92%   18.00%   +0.08%     
- Complexity    16154    16465     +311     
============================================
  Files          5939     5977      +38     
  Lines        533181   537728    +4547     
  Branches      65237    66025     +788     
============================================
+ Hits          95585    96840    +1255     
- Misses       426856   429968    +3112     
- Partials      10740    10920     +180
Flag Coverage Δ
uitests 3.52% <ø> (-0.15%) ⬇️
unittests 19.17% <0.00%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sudo87 sudo87 marked this pull request as ready for review February 23, 2026 06:44
@sudo87
Copy link
Copy Markdown
Contributor Author

sudo87 commented Feb 25, 2026

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Feb 25, 2026

@sudo87 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Feb 25, 2026

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16937

Copilot AI reviewed Feb 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an access control issue for non-public templates accessed by non-admin users. The fix ensures that ownership semantics are properly enforced by changing the sameOwner parameter from false to true in the access check for non-public templates.

Changes:

  • Fixed access control check for non-public templates to enforce ownership semantics by setting sameOwner=true instead of false
  • Minor code formatting change (consolidating else-if statement onto one line)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@winterhazel winterhazel self-requested a review February 27, 2026 13:52
@bernardodemarco bernardodemarco mentioned this pull request Mar 5, 2026
Merged
14 tasks
@winterhazel winterhazel added this to the 4.23.0 milestone Mar 28, 2026
@winterhazel
Copy link
Copy Markdown
Member

winterhazel commented Mar 28, 2026

@sudo87 is this change related to a specific issue/situation? This flag is just used to ensure that all the controlled entities provided to checkAccess are owned by the same account. As only a single template is provided, changing it should not affect anything.

@sudo87
Copy link
Copy Markdown
Contributor Author

sudo87 commented Mar 30, 2026

@sudo87 is this change related to a specific issue/situation? This flag is just used to ensure that all the controlled entities provided to checkAccess are owned by the same account. As only a single template is provided, changing it should not affect anything.

Hi @winterhazel, this change is based on how checkAccess is used here. Right now we pass sameOwner = false, which effectively relaxes the ownership check.
For a non-public template accessed by a non-admin, we should be enforcing that it belongs to the caller’s account. Setting sameOwner = true makes that explicit and aligns with the intended access control.

Please let me know if change makes sense.

@winterhazel
Copy link
Copy Markdown
Member

winterhazel commented Apr 6, 2026

@sudo87 is this change related to a specific issue/situation? This flag is just used to ensure that all the controlled entities provided to checkAccess are owned by the same account. As only a single template is provided, changing it should not affect anything.

Hi @winterhazel, this change is based on how checkAccess is used here. Right now we pass sameOwner = false, which effectively relaxes the ownership check. For a non-public template accessed by a non-admin, we should be enforcing that it belongs to the caller’s account. Setting sameOwner = true makes that explicit and aligns with the intended access control.

Please let me know if change makes sense.

@sudo87 have a look at what sameOwner is used for in the implementation of com.cloud.user.AccountManagerImpl#checkAccess(com.cloud.user.Account, org.apache.cloudstack.acl.SecurityChecker.AccessType, boolean, org.apache.cloudstack.acl.ControlledEntity...). It does not ensure that the controlled entity (the template) belongs to the calling account.

@sudo87
Copy link
Copy Markdown
Contributor Author

sudo87 commented Apr 7, 2026

Thanks @winterhazel for the hint, I misunderstood the flow.

Since both branches were effectively doing the same checkAccess call, I have consolidated them. Please review if changes are good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@weizhouapache weizhouapache Awaiting requested review from weizhouapache

@shwstppr shwstppr Awaiting requested review from shwstppr

@winterhazel winterhazel Awaiting requested review from winterhazel

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

4.23.0

Development

Successfully merging this pull request may close these issues.

4 participants

@sudo87 @blueorangutan @winterhazel